Skip to content

feat: allow user to pass multiple files to fix and check commands#119

Closed
sinrohit-desco wants to merge 1 commit intooppiliappan:masterfrom
sinrohit-desco:patch-1
Closed

feat: allow user to pass multiple files to fix and check commands#119
sinrohit-desco wants to merge 1 commit intooppiliappan:masterfrom
sinrohit-desco:patch-1

Conversation

@sinrohit-desco
Copy link
Copy Markdown

Resolves #118

@sinrohit-desco sinrohit-desco marked this pull request as draft August 14, 2025 06:27
@mightyiam
Copy link
Copy Markdown
Collaborator

I think we should have this. How about a test though?

@sinrohit-desco
Copy link
Copy Markdown
Author

I think we should have this. How about a test though?

Yeah, definitely but before that I want to rework the changes as the program needs to respect each .gitignore files.

Copy link
Copy Markdown

@TLATER TLATER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I much prefer this, I overlooked the Walker when I looked at this.

I'll close #96

Comment thread bin/src/dirs.rs Outdated
@sinrohit-desco
Copy link
Copy Markdown
Author

I completely implemented the iterator using Walk from ignore crate. Let me know your thoughts @TLATER @mightyiam
If everything looks good to you guys, I will add tests.

@sinrohit-desco sinrohit-desco requested a review from TLATER August 19, 2025 12:27
@sinrohit-desco sinrohit-desco marked this pull request as ready for review August 19, 2025 12:29
Copy link
Copy Markdown

@TLATER TLATER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good, much better than the existing code! Error handling might need some work though.

To be clear, since apparently this is less common parlance than I thought, "nit" means a small thing that I don't really think needs to be addressed but point out anyway because I'm overly specific about programming. So feel free to ignore those comments if what they're talking about doesn't also annoy you.

Comment thread bin/src/config.rs Outdated

use clap::Parser;
use lib::{LINTS, session::Version};
use lib::{session::Version, LINTS};
Copy link
Copy Markdown

@TLATER TLATER Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Spurious diff changes, it'd be best to revert that just for a clean commit history. Is this done by rustfmt? I didn't think import ordering is unstable?

Might as well squash the commits while you're at it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is happening because of pre-commit hook that runs when I try to commit.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-commit hooks run locally, you could disable it. But fair enough, guess there's a pre-commit hook but it didn't exist when this file was originally written. It's a nit for a reason, solve or don't as you see fit; personally I'd probably put this in a separate commit, but I sometimes take commit hygiene too far.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I will leave it up for the maintainer on how to proceed.

Comment thread bin/src/dirs.rs
Comment thread bin/src/dirs.rs Outdated
Comment thread bin/src/dirs.rs Outdated
Comment thread bin/src/dirs.rs Outdated
Comment thread bin/src/dirs.rs Outdated
Comment thread bin/src/dirs.rs Outdated
Copy link
Copy Markdown

@TLATER TLATER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, no further comments :) I'd love to resolve my earlier ones, sadly I don't have permissions to do that.

@mightyiam
Copy link
Copy Markdown
Collaborator

Sorry, I think #138 is the priority so we've been working on that.

@mightyiam
Copy link
Copy Markdown
Collaborator

Hey, #138 is complete. Sorry for the conflicts. Looking forward to reviewing this again.

@sinrohit-desco
Copy link
Copy Markdown
Author

Hey, #138 is complete. Sorry for the conflicts. Looking forward to reviewing this again.

Hi Shahar! I have rebased the PR and resolved the conflicts.

Copy link
Copy Markdown
Collaborator

@mightyiam mightyiam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this new feature include a test?

Comment thread bin/src/config.rs Outdated
};

use crate::{LintMap, dirs, err::ConfigErr, utils};
use crate::{dirs, err::ConfigErr, utils, LintMap};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do revert.

@voruti
Copy link
Copy Markdown

voruti commented Feb 20, 2026

I was just testing this out, but it's not compiling correctly.

error: this argument is passed by value, but not consumed in the function body
  --> bin/src/dirs.rs:15:22
error: variables can be used directly in the `format!` string
  --> bin/src/dirs.rs:43:23
  --> bin/src/dirs.rs:60:17

@sinrohit-desco
Copy link
Copy Markdown
Author

Apologies for leaving it unattended for this long.

I was just testing this out, but it's not compiling correctly.

error: this argument is passed by value, but not consumed in the function body
  --> bin/src/dirs.rs:15:22
error: variables can be used directly in the `format!` string
  --> bin/src/dirs.rs:43:23
  --> bin/src/dirs.rs:60:17

That's the clippy warning (error). I ran cargo build and didn't run clippy checks. Let me fix that and also rebase.

reimplement the iterator using ignore::Walk

This implementation simplifies things a bit also it returns a lazy
iterator which is memory efficient for large codebases.
@mightyiam
Copy link
Copy Markdown
Collaborator

Thank you for resuming. But, would you mind submitting against https://github.com/molybdenumsoftware/statix ?

@sinrohit-desco
Copy link
Copy Markdown
Author

Thank you for resuming. But, would you mind submitting against https://github.com/molybdenumsoftware/statix ?

Sure thing!!

@sinrohit-desco
Copy link
Copy Markdown
Author

Closing in favour of molybdenumsoftware#1921

Thank you for resuming. But, would you mind submitting against https://github.com/molybdenumsoftware/statix ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

statix check on multiple files

4 participants